Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calendar navigation #24124

Closed
wants to merge 4 commits into from
Closed

Calendar navigation #24124

wants to merge 4 commits into from

Conversation

zarend
Copy link
Contributor

@zarend zarend commented Dec 21, 2021

Draft PR of fixing focus and navigation issues on the calendar. Please see commit messages for details on these changes.

This is still a draft at least until I can do more testing across different browsers and screen readers.

Makes changes to the DOM structure of calendar cells for better screen reader experience. Previously, the DOM
structure looksed like this:

```
<!-- Existing DOM structure of each calendar body cell -->
<td
 role="gridcell"
 aria-disabled="false"
 aria-current="date"
 aria-selected="true"
 <!-- ... -->
>
 <!-- additional details ommited -->
</>
```

Using the `gridcell` role allows screenreaders to use table specific
navigation and some screenreaders would announce that the cells are
interactible because of the presence of `aria-selected`. However, some
screenreaders did not announce the cells as interactable and treated it
the same as a cell in a static table (e.g. VoiceOver announces element
type incorrectly angular#23476).

This changes the DOM structure to nest buttons
inside of a gridcell to make it more explicit that the table cells can
be interacted with and are not static content. The gridcell role is
still present, so table navigation will continue to work, but the
interaction is done with buttons nested inside the `td` elements.
The `td` element is only for adding information to the a11y tree and not used for visual purposes.

Updated DOM structure:

```
<td
  role="gridcell"
  style="display: contents;"
>
  <div
   role="button"
   aria-disabled="false"
   aria-current="date"
   aria-selected="true"
   <!-- ... -->
  >
   <!-- additional details ommited -->
  </div>
</td>
```

Fixes angular#23476
When a a date cell on the calendar recieves focus, set the active date
to that cell. This ensures that the active date matches the date with
browser focus.

Previously, we set the active date on keydown and click, but that was
problematic for screenreaders. That's because many screenreaders trigger
a focus event instead of a keydown event when using screenreader
specific navigation (VoiceOver, Chromevox, NVDA).

Fixes angular#23483
@zarend zarend added Accessibility This issue is related to accessibility (a11y) area: material/datepicker labels Dec 21, 2021
@zarend zarend requested a review from crisbeto December 21, 2021 02:56
@zarend zarend added the target: patch This PR is targeted for the next patch release label Dec 21, 2021
-->
<td *ngFor="let item of row; let colIndex = index" role="gridcell" class="mat-calendar-body-cell-wrapper">
<div
role="button"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we use a button element instead of a div with a role?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the draft implementation of it, it was the path of least resistance to use div role="button" because it doesn't affect the visual styles. We would need to override a few (not sure exactly how many) style rules to get a button element to look correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -31,7 +31,12 @@ $calendar-range-end-body-cell-size:
padding-right: $calendar-body-label-side-padding;
}

.mat-calendar-body-cell-wrapper {
display: contents;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? Also it seems this isn't supported in Safari.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, well, the MDN Compatibility table for display says there is some issues on Safari with unusual elements 🤔 . I tried this on Safari and it seems to render fine.

Safari Version 15.1 (17612.2.9.1.20)

.mat-calendar-body-cell {
display: table-cell;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had an issue where NVDA would announce an extra "table" because something had display: table. I wouldn't be surprised if it becomes an issue here too. See #23446.

@@ -122,6 +122,9 @@ export class MatCalendarBody implements OnChanges, OnDestroy {
/** Emits when a new value is selected. */
@Output() readonly selectedValueChange = new EventEmitter<MatCalendarUserEvent<number>>();

/** Emits when a new date becomes active. */
@Output() readonly activeValueChange = new EventEmitter<MatCalendarUserEvent<number>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this event used for? It seems like we're only emitting it/forwarding it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good question. What this is trying to accomplish is making it so that the calendar body can update the value of the activeDate when a cell receives the focus event. activeDate has a reference in three components – the root component MatCalendarBody, the component for the specific view month/year/multiyear and the calendar body.

This might be easier to do with banana in a box rather than passing an event. That is currently (in master branch) how the MatCalendar component gives the activeDate to the MatMonthView component (and also MatMonthView and MatMultiYearView).

[(activeDate)]="activeDate"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so if I'm understanding it correctly, what it's trying to achieve is to sync the activeDate back up with the calendar if focus is moved by something we don't control (e.g. a keyboard shortcut)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, if I had more time, I would even go as far as renaming activeDate to focusedDate, but I'm not sure that would be worth changing public API for.

-->
<td *ngFor="let item of row; let colIndex = index" role="gridcell" class="mat-calendar-body-cell-wrapper">
<div
role="button"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -149,13 +149,14 @@ describe('MatMonthView', () => {
fixture.detectChanges();
});

it('should decrement date on left arrow press', () => {
fit('should decrement date on left arrow press', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover from debugging?

@@ -122,6 +122,9 @@ export class MatCalendarBody implements OnChanges, OnDestroy {
/** Emits when a new value is selected. */
@Output() readonly selectedValueChange = new EventEmitter<MatCalendarUserEvent<number>>();

/** Emits when a new date becomes active. */
@Output() readonly activeValueChange = new EventEmitter<MatCalendarUserEvent<number>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so if I'm understanding it correctly, what it's trying to achieve is to sync the activeDate back up with the calendar if focus is moved by something we don't control (e.g. a keyboard shortcut)?

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@zarend
Copy link
Contributor Author

zarend commented Jan 7, 2022

@crisbeto thanks for looking this draft. Over the break, it was easier to work on multiple changes on the same branch until we started merging PR's again.

I'm closing this in favor of individual non-draft PR's for these changes. Please see:

@zarend zarend closed this Jan 7, 2022
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) area: material/datepicker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants